src/tools/send_file.rs (1)

40-63: Fail-open when workspace is None permits reading arbitrary files.

This was raised in a prior review. When no workspace is configured, validate_workspace_path returns Ok(path) for any input, allowing file exfiltration. Consider failing closed (returning an error) when workspace is unset, and updating all call sites to use with_workspace.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/tools/send_file.rs` around lines 40 - 63, validate_workspace_path
currently allows any path when self.workspace is None which enables file
exfiltration; change it to return a SendFileError when workspace is unset
(fail-closed) instead of returning Ok(path), and update all call sites to call
with_workspace (or otherwise ensure a workspace is provided) before invoking
validate_workspace_path so callers can no longer bypass workspace checks; refer
to the validate_workspace_path function and the SendFileError type and adjust
callers that currently rely on the None behavior to use with_workspace to supply
a workspace.

src/sandbox.rs (3)

120-123: PATH concatenation uses hardcoded : separator and std::env::var (lossy on non-UTF-8).

This was flagged in a prior review. Using std::env::split_paths/std::env::join_paths with var_os would be more robust.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/sandbox.rs` around lines 120 - 123, The PATH construction currently
builds path_env with std::env::var and a hardcoded ":" and so can fail for
non-UTF-8 PATHs and non-Unix separators; change it to use std::env::var_os +
std::env::split_paths and std::env::join_paths so you append self.tools_bin (as
a PathBuf/OsString) onto the existing PATH entries portably; update the code
that sets path_env (the variable created where self.tools_bin.display() is used)
to collect the existing var_os, push the tools_bin path, then join_paths to
produce an OsString/Path that you can pass into the child environment.

---

255-259: Past review: SBPL profile does not protect data_dir as read-only.

If workspace overlaps with data_dir on macOS, the profile currently allows writes to the data directory. This was flagged in a prior review.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/sandbox.rs` around lines 255 - 259, When building the SBPL profile in
sandbox.rs where profile.push_str adds the workspace writable rule, ensure the
data_dir is explicitly protected by denying writes to it before allowing
workspace writes: reference the profile construction code that uses workspace
and data_dir variables and add a deny file-write* (subpath "<data_dir>") entry
(or otherwise constrain the workspace allow to exclude data_dir) so that macOS
profiles cannot grant write access to data_dir when workspace overlaps it.

---

163-168: Past review: canonicalize workspace/writable_paths before passing to bwrap.

Symlinked paths can unintentionally expand the writable boundary, and missing paths will cause spawn failures. This was flagged previously.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/sandbox.rs` around lines 163 - 168, self.workspace and entries in
self.writable_paths are passed raw to bwrap, which lets symlinks escape the
intended sandbox and fails on missing paths; canonicalize the paths before
adding bind args: call std::fs::canonicalize on self.workspace and each writable
in the code that builds cmd (the block that uses
cmd.arg("--bind").arg(&self.workspace)... and the for writable in
&self.writable_paths loop), handle errors by either creating missing directories
or returning a descriptive error, dedupe canonical paths (e.g. with a HashSet)
to avoid duplicate binds, and use the canonicalized path values for both source
and destination when invoking cmd.arg("--bind").

src/config.rs (1)

3762-3762: ⚠️ Potential issue | 🟠 Major

Hot-reload updates config but not the active Sandbox instance.
Line 3762 swaps the SandboxConfig, but the per-agent Sandbox used by tools is constructed elsewhere at startup and isn’t rebuilt. If you expect live changes to take effect, rebuild/swap the sandbox (and downstream tool servers) on reload, or make sandbox config explicitly non-reloadable.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/config.rs` at line 3762, The reload currently only swaps the
SandboxConfig via self.sandbox.store(Arc::new(resolved.sandbox)); but does not
rebuild the runtime per-agent Sandbox instances or notify tool servers; update
the reload path to (1) construct new Sandbox instances from the new
SandboxConfig (use the same constructor(s) used at startup—locate where Sandbox
is created/initialized) and atomically swap those active Sandbox references for
each agent, and (2) propagate the new Sandbox to downstream tool servers (call
their reload/update methods or restart them) so they use the new instance;
alternatively, if dynamic swapping is not desired, make SandboxConfig
non-reloadable by reverting the store call and emitting a clear log/warning that
Sandbox changes require restart.

docs/content/docs/(features)/workers.mdx (1)

146-158: The identity-file guard implementation is correct; consider clarifying that sandbox isolation covers filesystem containment only.

The guard at src/tools/file.rs:206–215 correctly blocks writes to SOUL.md, IDENTITY.md, and USER.md, with an error message directing users to the identity management API. The workspace boundary validation in resolve_path() (lines 26–55) is also in place. The documentation is accurate.

For clarity: consider a brief note that sandboxing covers only filesystem containment (read-only host FS, writable workspace) and doesn't include network or CPU isolation, so readers don't infer full system isolation.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/content/docs/`(features)/workers.mdx around lines 146 - 158, The docs
state sandboxing behavior but can be misread as full system isolation; update
the "Sandbox and Protected Paths" section to explicitly state that sandbox
isolation covers filesystem containment only (read-only host FS, writable
workspace, sandbox-specific writable_paths) and does NOT provide network, CPU,
or other resource isolation; reference the file-tool guard (blocks SOUL.md,
IDENTITY.md, USER.md), resolve_path(), and exec tool environment-variable checks
so readers know which protections exist.

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

src/sandbox.rs (2)

262-325: SBPL profile paths are not escaped — injection risk from paths containing double quotes.

Paths from workspace, writable_paths, and data_dir are interpolated directly into SBPL string literals via format!. A path containing " would break SBPL syntax and could inject arbitrary rules. While these are admin-configured, defense-in-depth warrants sanitization.

Also, canonicalize_or_self is called again in generate_sbpl_profile for paths that were already canonicalized in Sandbox::new. This is harmless but redundant.

Proposed helper

+/// Escape a path for safe inclusion in an SBPL string literal.
+fn sbpl_escape(path: &Path) -> String {
+    path.display()
+        .to_string()
+        .replace('\\', "\\\\")
+        .replace('"', "\\\"")
+}

Then use sbpl_escape(&workspace) instead of workspace.display() in all SBPL interpolations.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/sandbox.rs` around lines 262 - 325, The SBPL profile inserts filesystem
paths directly in generate_sbpl_profile which allows a path with double quotes
to break SBPL syntax; add a helper like sbpl_escape(path: &Path) -> String that
returns the path string with double quotes and any SBPL-special characters
escaped (use it when formatting strings for workspace, each entry in
writable_paths, data_dir, and tmp), replace all calls that use
workspace.display()/canonical.display() with sbpl_escape(&canonical) and avoid
redundant canonicalize_or_self calls if paths are already canonicalized earlier
(or at least call canonicalize_or_self once per path and reuse the result) so
all interpolated path values are safely quoted/escaped before being inserted
into the SBPL profile.

---

355-367: /usr/bin/true is not portable to all Linux distributions.

On Alpine, Busybox-based, or other minimal distributions, true may live at /bin/true or be a shell builtin only. A failed inner command would make proc_supported report false for the wrong reason.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/sandbox.rs` around lines 355 - 367, The preflight bwrap invocation
hardcodes "/usr/bin/true" which is not portable; change the inner command passed
to Command::new("bwrap") in sandbox.rs (the proc_check call) to use a portable
executable name (e.g. "true" without an absolute path) or try a small fallback
resolution (attempt "/usr/bin/true" then "/bin/true" or use "true" so PATH
lookup can find a distro-specific location) so a missing hardcoded path doesn't
cause proc_supported to be misreported; update the args array passed to
Command::new("bwrap") accordingly (the place that currently contains "--",
"/usr/bin/true").

tests/context_dump.rs (1)

340-340: bootstrap_deps() is duplicated between tests/bulletin.rs and tests/context_dump.rs.

Both test files contain a near-identical ~80-line bootstrap_deps() function. Consider extracting it into a shared test helper (e.g., a tests/common/mod.rs or a test_support module) to reduce maintenance burden when fields like sandbox are added to AgentDeps.

Also applies to: 495-495

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/context_dump.rs` at line 340, Extract the duplicated bootstrap_deps()
helper into a shared test helper module (e.g., tests/common/mod.rs or
tests/test_support.rs), move the existing implementation (including construction
of AgentDeps and usage of deps.sandbox) into that module as a single pub
function, replace the duplicate definitions in tests/context_dump.rs and
tests/bulletin.rs with a use/import of the shared bootstrap_deps, and update any
test imports so they call the shared function; ensure the shared helper exports
the same signature and uses AgentDeps so changes to AgentDeps (like adding
sandbox) only need updating in one place.

src/tools/send_file.rs (1)

38-40: Asymmetric canonicalization: workspace falls back silently while file path must resolve.

If self.workspace can't be canonicalized (e.g., it doesn't exist yet), the fallback to the raw path means starts_with compares a fully resolved file path against a potentially unresolved workspace path. This is fail-closed (legitimate files get rejected with a confusing "ACCESS DENIED" error), not fail-open, so it's safe — but it could be surprising at runtime.

Consider logging a warning when workspace canonicalization fails, similar to how sandbox.rs handles writable_paths:

Proposed improvement

-        let workspace_canonical = workspace
-            .canonicalize()
-            .unwrap_or_else(|_| workspace.clone());
+        let workspace_canonical = workspace.canonicalize().unwrap_or_else(|error| {
+            tracing::warn!(
+                workspace = %workspace.display(),
+                %error,
+                "send_file: workspace canonicalization failed, using raw path"
+            );
+            workspace.clone()
+        });

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/tools/send_file.rs` around lines 38 - 40, The workspace canonicalization
currently falls back silently to the original path (let workspace_canonical =
workspace.canonicalize().unwrap_or_else(|_| workspace.clone());) which can cause
comparisons like starts_with to mismatch against fully-resolved file paths;
update the logic in send_file.rs to attempt canonicalize on self.workspace but
log a warning when canonicalize fails (including the error and the original
workspace string) before using the fallback, mirroring the approach in
sandbox.rs for writable_paths, so callers see why comparisons may behave
unexpectedly.

src/sandbox.rs (2)

333-343: cfg!(target_os = ...) compiles to a constant — both branches are compiled on all platforms.

This is fine for correctness (the unused branch is dead code that the compiler will optimize away), but worth noting: if detect_bubblewrap or detect_sandbox_exec ever gain platform-specific dependencies, you'd need #[cfg(target_os = "...")] attributes instead.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/sandbox.rs` around lines 333 - 343, The current detect_backend() uses
runtime cfg!(target_os = "...") which compiles both branches; change to
conditional compilation so platform-specific functions aren’t compiled on other
OSes: annotate the detect_bubblewrap() call with #[cfg(target_os = "linux")] and
the detect_sandbox_exec() call with #[cfg(target_os = "macos")], and provide a
#[cfg(not(any(target_os = "linux", target_os = "macos")))] fallback that logs
and returns SandboxBackend::None; keep function name detect_backend and calls
detect_bubblewrap/detect_sandbox_exec so locating the changes is
straightforward.

---

265-278: (allow process-exec) without path restriction allows executing any binary on the system.

The SBPL profile grants unrestricted process-exec, meaning any binary readable on the host can be executed inside the sandbox. While filesystem writes are contained, this allows running tools like curl, nc, python etc. for data exfiltration over the network (since (allow network*) is also granted). This is consistent with the PR's stated scope (no network isolation), but worth documenting as a known limitation in the profile comments.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/sandbox.rs` around lines 265 - 278, The SBPL profile currently includes
an unrestricted (allow process-exec) entry inside the profile string (the
`profile` variable) which permits executing any readable host binary; update the
profile text to include a short comment adjacent to the (allow process-exec)
line documenting this known limitation and the associated risk (e.g., ability to
run host tools like curl/python for exfiltration when network is allowed), or—if
intended—replace it with a path-restricted form (e.g., restricting to specific
runtime binaries) and mention that choice in the comment so reviewers understand
the trade-off.

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro